-
Notifications
You must be signed in to change notification settings - Fork 16
feat(Runtime): Add Rollout Percentage functionality #81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hello! @chiraag918 Although I haven’t received any direct requests for it yet, I believe there are definitely use cases where the progressive rollout feature will be useful. -- About the featureThe idea of using the device ID for bucketing is very interesting! I’ve been thinking about how we could add a bit of randomness to this. -- About the
|
Hello @floydkim First of all, thank you so much for taking the time to review my contribution. I really appreciate your thoughtful feedback! 🙏 Latest Commit Changes - followed your suggestion to include packageHash About the disabled flag About testing Thanks again for the valuable feedback! Please let me know if you'd like me to make any further changes. |
@chiraag918 I noticed that you added a random factor along with the package hash. I also think that adding a random factor somewhat diminishes the meaning of using the device ID and package hash. With the random factor added, doesn’t the outcome essentially become the same as just using Math.random()? 🤔 If the random factor were removed, I believe deterministic bucketing could be achieved for the same device and the same CodePush update. (If my previous comment using the phrase “a bit of randomness” caused any confusion, I sincerely apologize. 🙏) |
@floydkim true, if the user were to uninstall & reinstall the app, the previous decision can get changed depending on the random factor 😅. So in that case, keeping device ID and packageHash would solve for this use case right? I added the random factor to make it a bit more random. I can remove the random multiplier. Any other suggestions or thoughts? (No worries, these conversations helps me with understanding these corner cases, thanks 😀) |
@chiraag918 If the need ever arises, we could later add an option to make the bucketing completely random. :) |
@floydkim I've removed the random factor multiplier in my recent commit. Request you to please review the PR once. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @chiraag918 !
Sorry for the delay.
I reviewed the code thoroughly and had some questions, so I’ve asked them.
Please respond when you have time. 😄
Once I understand your answers, I’m thinking of merging the PR first and then making the changes myself, including some minor coding style tweaks.
What do you think about it?
I want to reduce the back-and-forth on feedback and move quickly toward releasing the feature.
typings/react-native-code-push.d.ts
Outdated
* @param deploymentKey The deployment key to use to query the CodePush server for an update. | ||
* | ||
* @param handleBinaryVersionMismatchCallback An optional callback for handling target binary version mismatch | ||
*/ | ||
function checkForUpdate(handleBinaryVersionMismatchCallback?: HandleBinaryVersionMismatchCallback): Promise<RemotePackage | null>; | ||
function checkForUpdate(deploymentKey?: string, handleBinaryVersionMismatchCallback?: HandleBinaryVersionMismatchCallback): Promise<RemotePackage | null>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since deploymentKey has been deprecated, it should not be added back.
Was this change intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry took this change from the package code that I was using to test my changes. Have reverted this change.
CodePush.js
Outdated
if(remotePackage?.skipRollout){ | ||
syncStatusChangeCallback(CodePush.SyncStatus.UNKNOWN_ERROR); | ||
return CodePush.SyncStatus.UNKNOWN_ERROR; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this status should be CODEPUSH_SKIPPED.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this
CodePush.js
Outdated
const remotePackage = { ...update, ...PackageMixins.remote() }; | ||
const remotePackage = { ...PackageMixins.remote(), ...update }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to change the order of the spread operation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I don't think so, initially I guess it was taking default rollback value as 100, hence, had changed the order, so the rollout value comes from the remote fetched config. Now I've removed the fallback value as suggested in one of the other other PR comments. Reverted this change.
CodePush.js
Outdated
// Rollout filtering | ||
const shouldApply = await shouldApplyCodePushUpdate(remotePackage, nativeConfig.clientUniqueId, sharedCodePushOptions?.onRolloutSkipped); | ||
|
||
if(!shouldApply && !remotePackage.enabled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enabled
property is not exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes my bad, actually I wanted to know when the package wasn't enabled and based on that I skip handling the rollout part. This is already handed in the above if condition where if update is null/undef it skips the else block inside which rollout logic is present. So !remotePackage.enabled is no longer required.
if(prevRolloutCacheKey) | ||
await RolloutStorage.removeItem(prevRolloutCacheKey); | ||
|
||
await RolloutStorage.setItem(ROLLOUT_CACHE_KEY, rolloutKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems possible to implement without adding ROLLOUT_CACHE_KEY
..
I will take a look while refactoring the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay pls do share once you refactor.
CodePush.js
Outdated
} | ||
|
||
function getRolloutKey(label, rollout) { | ||
return `${ROLLOUT_CACHE_PREFIX}${label}_rollout_${rollout ?? 'full'}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When rollout parameter is 100, returns ${ROLLOUT_CACHE_PREFIX}${label}_rollout_${100}
,
and when it’s undefined, returns ${ROLLOUT_CACHE_PREFIX}${label}_rollout_${'full'}
.
I don’t think the two return values need to be different, and it seems fine to just use 100 directly in the key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have made this change.
CodePush.js
Outdated
if(remotePackage?.skipRollout){ | ||
syncStatusChangeCallback(CodePush.SyncStatus.UNKNOWN_ERROR); | ||
return CodePush.SyncStatus.UNKNOWN_ERROR; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason why we need to distinguish the state where it’s skipped because it’s not within the rollout range?
In apps that used the UP_TO_DATE state to inform users that "the app is up to date", code changes will be necessary due to the addition of this new state.
(If not using the rollout feature, no changes would be needed, of course.)
There may be operational or management reasons, but I think it’s possible to collect that information through the onRolloutSkipped callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a different state to maintain a different state for codepush skipped scenario for better readability during dev. Instead what would you suggest to keep this state as?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, if the update is not the rollout target, it could be treated the same as the UP_TO_DATE state.
Distinguishing between the two cases can be helpful for understanding the code and flow.
But from an end user’s perspective, they are essentially the same and app should present a message like 'The app is already up to date.'
For developers using our library, I feel it might be more natural to handle this with just the UP_TO_DATE state.
This way, they do not have to account for both states in their if statements to display the same message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay makes sense, have changed this state to UP_TO_DATE and have removed refs for CODEPUSH_SKIPPED.
import com.facebook.react.bridge.ReactMethod; | ||
import com.facebook.react.bridge.Promise; | ||
|
||
public class RolloutStorageModule extends ReactContextBaseJavaModule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SettingsManager already handles interactions with SharedPreference.
Was the reason you added a new module instead to separate concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't really want to touch the other modules, hence, added this module, but lmk, if I should instead stick to using SettingsManager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, no. I was just curious about the reason.
For now, since I’m not familiar with the implementation of SettingsManager, I like the way you implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay great!
cli/commands/releaseCommand/index.js
Outdated
@@ -16,6 +16,7 @@ program.command('release') | |||
.option('-j, --js-bundle-name <string>', 'JS bundle file name (default-ios: "main.jsbundle" / default-android: "index.android.bundle")') | |||
.option('-m, --mandatory <bool>', 'make the release to be mandatory', parseBoolean, false) | |||
.option('--enable <bool>', 'make the release to be enabled', parseBoolean, true) | |||
.option('--rollout <number>', 'rollout percentage (0-100)', parseFloat, 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think it would be better not to add the rollout property when the rollout feature is not used, so that the JSON data doesn’t grow unnecessarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes done removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was a misunderstanding because I didn’t explain clearly. Sorry.
What I meant was that undefined is fine as the default value, not that the CLI option should be removed. 😭
If the user doesn’t pass the --rollout option when calling the release command, I’d prefer that the release history object not include the rollout property when it’s created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops my bad. I've added this cli cmd option with the default value argument removed.
ios/CodePush/RolloutStorage.h
Outdated
RCT_EXTERN_METHOD(setItem:(NSString *)key value:(NSString *)value) | ||
RCT_EXTERN_METHOD(getItem:(NSString *)key resolver:(RCTPromiseResolveBlock)resolve rejecter:(RCTPromiseRejectBlock)reject) | ||
RCT_EXTERN_METHOD(removeItem:(NSString *)key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure if the RCT_EXPORT_METHOD macro is needed in the header file as well.
From what I can find, it doesn’t seem to be commonly declared in headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm afaik its a good practice to have a header file to have function prototype declarations, hence, added it. Should I remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not an iOS/Objective-C expert, so I'd like to follow either the library’s current implementation style or approaches used in other libraries.
In the CodePush.h and CodePush.m files, the RCT_EXPORT_METHOD macro appears only in the .m file, and the header doesn’t declare these methods.
(I’m not certain if this is because the macro removes the need for function prototype declarations.. 🤔)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, RCT_EXPORT_METHOD need not be included in the header files. The ones that you shared seems to be like react bridging header files to bridge objective-c & swift code, since, swift doesn't yet support exposing native methods to React JS like how we do in objective-c.
- Similarly RCT_EXTERN_METHOD is similar to RCT_EXPORT_METHOD but used with swift files. Including/Excluding it in our case doesn't make a difference is what I suppose.
- Yes you're right, in CodePush.h and CodePush.m files, the RCT_EXPORT_METHOD macro appears only in the .m file, since its not required to duplicate the macro in both the files.
But considering both 1 & 2, I've removed RCT_EXTERN_METHOD from the .h file and RCT_EXPORT_METHOD only is used in the .m file, as this is what RN suggests: https://reactnative.dev/docs/0.73/native-modules-ios#create-custom-native-module-files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I confused RCT_EXTERN_MODULE with RCT_EXPORT_MODULE.
(And also RCT_EXTERN_METHOD and RCT_EXPORT_METHOD)
I wasn’t familiar with RCT_EXTERN_MODULE, but as you mentioned, it’s used to register implementations in Swift.
Thanks for sharing your knowledge.
Anyway, the final code you’ve updated looks good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay thanks
Sorry was little busy with work, hence the delay. I've addressed the PR comments. I think 3 comments, I wasn't sure, need your suggestion on it. Pls go through them and lmk, will take action. Thank you!!! |
@chiraag918 |
@floydkim Addressed the last few open comments. Pls check it out and lmk if any other changes are required. Thanks for your time & support! 🤝 |
@chiraag918 Thank you for your great work. I'll slightly update the PR title for the GitHub release notes and then merge it. |
@floydkim Thank you so much for your time, effort, for guiding this through and accepting my contribution. I really appreciate the collaboration. Please feel free to reach out if anything comes up during your testing or refactoring, I’ll be happy to help with any issues 😁 |
✨ Feature: Rollout Percentage with Persistent Bucketing & Release Awareness
📌 What this PR adds
This PR introduces client-side rollout percentage support for CodePush updates, with the following features:
Rollout-based user bucketing
rollout
percentage (e.g.,30
), each device is deterministically assigned to a 0–99 bucket using a stable hash of its unique ID.Persistent rollout decisions per update
packageHash
androllout
), the decision is cached locally using AsyncStorage.Rollout re-evaluation on update change
label
or therollout
value changes, the rollout decision is recalculated and the cache is reset automatically.🧪 Behavior Summary
✅ Why this is useful
🙏 Request to Maintainers
Dear maintainers, I'd love your feedback on this PR. If this feature aligns with your goals for the package, I kindly request a review and merge.
rollout
.Thank you for maintaining this awesome package! 🙌
Let me know if you'd like me to add: